I think, this changes are completely incorrect. Before this changes, error returns when some checks are fail, now error returns always, because you remove this checks.

@ForesterDev Yes I was very tired at that moment, I can fix that.

Tests performed: (Does it build, tested in-game, etc.)
None. Feedback please?

What do you mean with "none" ? No tests at all ?

No tests at all, obviously as I did it wrong the first time. It's a pretty straight forward change, the question is does it break anything else related, and I can't think of anything. I can also change it to increase the distance rather than remove the check completely.

for sure the check is to stop hacks.

4 * INTERACTION_DISTANCE would not be enough here?

@Aokromes Yes I thought the check might be an anti-hack. I can test at least one spot affected by this and find out if 4 * INTERACTION_DISTANCE is enough for it.

And now it will always fail with flight masters (npc != nullptr)

edit: 4 * INTERACTION_DISTANCE is almost enough but maybe someone else could still find a different way to fix this. Closing for now, if no one else does anything I'll try again later after testing.

Meh i checked on 4.3.4 db and it got diff coordinates, then compared with mangos, we have same coords, checked sniff, bad luck Steward of Time is on proper coords.

Tested with 6 * INTERACTION_DISTANCE, fixes #18615.

feels like hack

please include the original commit that set the value to 2 in your PR

Why do we even check distance from taxi node here instead of distance from npc (if present)?

@jackpoz - 2 was the original value
@Treeston how would you find the npc's position?

npc is passed to ActivateTaxiPathTo

I tried this:

if (node->map_id != GetMapId() || !IsInDist(npc->GetPositionX(), npc->GetPositionY(), npc->GetPositionZ(), 2 * INTERACTION_DISTANCE))

which causes a server crash. Any idea on how to fix that?

npc can be null

@Treeston I need a new hint.

if you are doing this within

bool Player::ActivateTaxiPathTo(std::vector<uint32> const& nodes, Creature* npc /*= nullptr*/, uint32 spellid /*= 0*/)

, you check that npc is not null, either if (npc) or if (!npc) \ return false; and go from there.

npc == null should still be handled (same check as current).

Please don't give advice if you have no idea what you are talking about.

I guess it should be like this

diff --git a/src/server/game/Entities/Player/Player.cpp b/src/server/game/Entities/Player/Player.cpp
index 65e7ec8..cf6e6ad 100644
--- a/src/server/game/Entities/Player/Player.cpp
+++ b/src/server/game/Entities/Player/Player.cpp
@@ -21114,16 +21114,19 @@ bool Player::ActivateTaxiPathTo(std::vector<uint32> const& nodes, Creature* npc
     }
 
     // check node starting pos data set case if provided
-    if (node->x != 0.0f || node->y != 0.0f || node->z != 0.0f)
+    if (!npc)
     {
-        if (node->map_id != GetMapId() || !IsInDist(node->x, node->y, node->z, 2 * INTERACTION_DISTANCE))
+        if (node->x != 0.0f || node->y != 0.0f || node->z != 0.0f)
         {
-            GetSession()->SendActivateTaxiReply(ERR_TAXITOOFARAWAY);
-            return false;
+            if (node->map_id != GetMapId() || !IsWithinDist3d(node->x, node->y, node->z, 2 * INTERACTION_DISTANCE))
+            {
+                GetSession()->SendActivateTaxiReply(ERR_TAXITOOFARAWAY);
+                return false;
+            }
         }
     }
     // node must have pos if taxi master case (npc != NULL)
-    else if (npc)
+    else

@Eliminationzx That fixed the crash, but now comes with the out of range error again (Still testing on Steward of Time) I believe it's because the npc doesn't actually start the taxi, the SAI involved makes the player self-cast the taxi start spell, so there's really no NPC involved in that calculation.

@Staleness89, post updated.

@Eliminationzx still doesn't seem to work.

it was working for years, and i don't remember any commit moving it, also another mob on dragonbright have the same issue.

Would be better to actually debug the root cause for the problem instead of touching code that isn't related to the problem being there in first place.

I retract. Maximum interaction distance tolerance is what caused the issue in the first place:

Commit 8758448 changed maximum distance from sqrt((2 * INTERACTION_DISTANCE)³) ie 31.62 yds approx to (2 * INTERACTION_DISTANCE) ie 10 yds

8758448 was an intentional change. Old limit was practically non-existant, and open to hacks.

Simply increasing the antihack check distance is not the correct fix here.

so.. where should it be fixed if not here?

I know, but it's too restrictive

It isn't. The issue here (as discussed in #15398) is that the core compares against the wrong position.

I miscalculated, squared distances compared against squared constant, so max distance was sqrt(1000) instead 1000 ie 31.62 yds. Updated my previous comment

Still a typo, though.

No. "Proper" distance check based on intent of old code would be sqrt(2^2+2^2+2^2), or 3.46, times INTERACTION_DISTANCE, if you really cared.

Realistically you'll never be off in more than two directions so it'd be more of 2.82*INTERACTION_DISTANCE.

INTERACTION_DISTANCE is 5 yds, those are multiplications, (2*5)*(2*5)*(2*5) = 10*10*10 = 1000. Compared squared distance against a squared distance ie sqrt(1000).

I don't know how much leeway allows the client, if any.

Can we do something like this?

diff --git a/src/server/game/Entities/Player/Player.cpp b/src/server/game/Entities/Player/Player.cpp
index 65e7ec8..394502f 100644
--- a/src/server/game/Entities/Player/Player.cpp
+++ b/src/server/game/Entities/Player/Player.cpp
@@ -21114,20 +21114,23 @@ bool Player::ActivateTaxiPathTo(std::vector<uint32> const& nodes, Creature* npc
     }
 
     // check node starting pos data set case if provided
-    if (node->x != 0.0f || node->y != 0.0f || node->z != 0.0f)
+    if (!spellid)
     {
-        if (node->map_id != GetMapId() || !IsInDist(node->x, node->y, node->z, 2 * INTERACTION_DISTANCE))
+        if (node->x != 0.0f || node->y != 0.0f || node->z != 0.0f)
         {
-            GetSession()->SendActivateTaxiReply(ERR_TAXITOOFARAWAY);
+            if (node->map_id != GetMapId() || !IsInDist(node->x, node->y, node->z, 2 * INTERACTION_DISTANCE))
+            {
+                GetSession()->SendActivateTaxiReply(ERR_TAXITOOFARAWAY);
+                return false;
+            }
+        }
+        // node must have pos if taxi master case (npc != NULL)
+        else if (npc)
+        {
+            GetSession()->SendActivateTaxiReply(ERR_TAXIUNSPECIFIEDSERVERERROR);
             return false;
         }
     }
-    // node must have pos if taxi master case (npc != NULL)
-    else if (npc)
-    {
-        GetSession()->SendActivateTaxiReply(ERR_TAXIUNSPECIFIEDSERVERERROR);
-        return false;
-    }

There is no way to associate the caster with the npc triggering the event, as it's the same player. Should it be better to actually drop the check entirely if activating taxi through a spell?